AI Image-to-3D (TripoSR/ONNX) — full feature, all slices #765–#769 (#764) - #785
Merged
Conversation
…export proof Slice A of epic #764 (single-image → 3D via TripoSR/ONNX). De-risk spike: prove the two epic-level unknowns before building any user surface. Marching cubes (the codebase had no iso-surface code): - src/MarchingCubes.{h,cpp}: native Lorensen–Cline MC, pure-data (no Ogre/GL), public-domain edge+triangle tables, edge-hash vertex welding. Zero new deps (matches the SkinWeights #402 / QuadRetopo #401 native stance). Registered in SRC_FILES. - src/MarchingCubes_test.cpp: sphere/box/empty SDF proofs. Sphere is watertight (0 boundary edges, 0 non-manifold, Euler χ=2), on-surface to 0.0004 vs a 0.043 cell; box AABB exact. (Verified via standalone compile — UnitTests needs GL, which macOS test_main can't init; runs on Linux CI.) TripoSR ONNX export (proven against the real stabilityai/TripoSR weights): - scripts/export-triposr-onnx.py: offline dev tool (not shipped/wired). Splits the pipeline into an encoder (image[1,3,512,512] → scene_codes[1,3,40,64,64], ~1.68GB) and a per-point decoder (scene_codes+points[1,P,3] → density[1,P,1],color[1,P,3], ~180KB). Both export at opset 17; the decoder's grid_sample traces + runs under ORT 1.20.1 (round-trip match=True). Documents the pins/monkeypatch needed (transformers==4.35.0, torchmcubes stub, frozen ViT pos-encoding). C++ load-proof: - src/MeshGenSpike_test.cpp: ENABLE_ONNX-guarded, opens both graphs with the exact UniRigPredictor Ort::Session setup (ORT_ENABLE_ALL, CoreML EP, wide-string path) and asserts the I/O contract; skips until models are in the AppData cache. Compiles + links against real ORT headers. Verified end-to-end on macOS via a standalone build (decoder ran: density=[1,512,1]). Docs: docs/IMAGE_TO_3D_SPIKE_764.md (tensor contract, MC choice, risks, GO/NO-GO — GO) + TripoSR MIT-code+weights section in THIRD_PARTY_AI_MODELS.md. Go/No-Go: GO. Both risks retired — MC is correct + permissive, the network exports to ONNX cleanly with no autoregressive loop. Proceed to slice B (#766). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete image-to-3D generation feature (TripoSR via ONNX): native marching-cubes extraction, ONNX export/upload scripts, background removal preprocessing, a MeshGenPredictor inference engine, Ogre mesh building, and CLI ( ChangesImage-to-3D Mesh Generation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLIOrMCPOrGUI as CLI/MCP/GUI
participant MeshGenPredictor
participant BackgroundRemover
participant ONNXRuntime
participant MarchingCubes
participant MeshGenBuilder
User->>CLIOrMCPOrGUI: request image-to-3D generation
CLIOrMCPOrGUI->>MeshGenPredictor: ensureModelBlocking / predict(image, opts)
MeshGenPredictor->>BackgroundRemover: removeBackground (optional)
MeshGenPredictor->>ONNXRuntime: run encoder -> scene_codes
MeshGenPredictor->>ONNXRuntime: run decoder over grid -> densityField
MeshGenPredictor->>MarchingCubes: extract(densityField)
MarchingCubes-->>MeshGenPredictor: Mesh
MeshGenPredictor-->>CLIOrMCPOrGUI: Result(vertices, triangles, colors)
CLIOrMCPOrGUI->>MeshGenBuilder: buildSceneNode(result)
MeshGenBuilder-->>CLIOrMCPOrGUI: Ogre SceneNode
CLIOrMCPOrGUI-->>User: mesh loaded/exported
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6af8f46f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
…LI/MCP/GUI Slices B+C+D of epic #764. Builds a working single-image → 3D mesh path on top of the slice-A spike (native marching cubes + proven TripoSR ONNX export). The feature is now usable end-to-end in the app. Slice B — MeshGenPredictor (src/MeshGenPredictor.{h,cpp}), the 5th ONNX consumer: - Ogre-free + unit-tested. Runs the two exported TripoSR graphs — encoder image[1,3,512,512]→scene_codes[1,3,40,64,64], per-point decoder scene_codes+points[1,P,3]→density[1,P,1],color[1,P,3] — tiling the resolution^3 grid through the decoder in chunks, then MarchingCubes on (density-threshold). - Model paths/download plumbing cloned from UniRigPredictor (AppData/ai_models/ triposr/, QTMESH_TRIPOSR_MODEL_BASE_URL / ai/triposrModelBaseUrl override, QTMESH_TRIPOSR_NO_DOWNLOAD guard). ENABLE_ONNX-guarded; no fallback (generative). - buildGridPoints is x-fastest to match MC's field[z*n*n+y*n+x]. Tests cover it + the graceful no-model/no-ONNX paths; inference test skips without the model. Slice C — MeshGenBuilder (src/MeshGenBuilder.{h,cpp}), the only Ogre-touching part: - float arrays → Ogre::Mesh with accumulated per-vertex normals + optional DIFFUSE vertex color, 16-/32-bit index buffer by vertex count, bounds+load. Attaches via Manager::createEntity and returns the SceneNode for MeshImporterExporter::exporter. Slice D — the three surfaces: - CLI: `qtmesh generate3d <image> [-o out.glb] [--resolution 16..512] [--no-color]` (CLIPipeline::cmdGenerate3d) + dispatcher + recognized-subcommand list. - MCP: generate_mesh_from_image (MCPServer::toolGenerateMeshFromImage, ONNX-guarded schema, heavy tool). - GUI: Tools → "Generate 3D from Image…" → AIAssistManager::generateMeshFromImage (+ meshGenStarted/Completed/Error signals; QML_SINGLETON facade). All degrade gracefully (clear message, no crash) without ONNX/model; Sentry breadcrumb ai.assist.image_to_3d on each surface. Verified end-to-end on macOS with the locally-exported models: chair.png → 12,992 verts / 25,984 tris (+vertex color) → glb, round-trips cleanly (qtmesh info matches); robot.png at res 256 → 93,702 verts (exercises the 32-bit index path). Model hosting + dedicated tests/docs polish are slice #769. CLAUDE.md: generate3d examples + recognized-subcommand entry + architecture note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fernandotonon
changed the title
…removal
Addresses three issues found while testing image-to-3D in the app.
1. Inverted normals — MarchingCubes emitted triangles wound for an inside-NEGATIVE
field, but the extractor treats `v >= iso` as inside (inside-positive), so faces
pointed inward and generated meshes rendered inside-out. Flip the emit to
v0,v2,v1 so faces point outward (matches the header's documented winding). Added
a sphere winding assertion (100% of face normals now point outward; was 0%).
2. Model lay on its back — TripoSR's output frame vs +Y-up. MeshGenBuilder now bakes
a -90° X rotation into positions+normals so the mesh stands upright (baked into
vertex data, not a node transform, so it survives glTF export in any viewer).
3. Background removal — new BackgroundRemover (src/BackgroundRemover.{h,cpp}), the
6th ONNX consumer, runs U²-Net (Apache-2.0, the model rembg ships) to isolate the
subject before the encoder — TripoSR needs a clean background. Input
[1,3,320,320] ImageNet-normalized → [1,1,320,320] saliency → composited over
white; bilinear-upsampled mask with a soft feather; falls back to the raw image
if the model/ONNX is absent or the mask keeps too little. Model
AppData/ai_models/rembg/u2net.onnx (env/QSettings override + no-download guard).
Wired as MeshGenPredictor::Options::removeBackground → CLI --remove-bg, MCP
remove_bg, and ON by default in the GUI (the file picker takes arbitrary photos).
Verified end-to-end: normals outward (standalone test), chair stands upright
(turntable), and an object-on-busy-background image reconstructs cleaner with
--remove-bg (9,966 vs 12,658 verts). Docs: CLAUDE.md + THIRD_PARTY_AI_MODELS.md
(U²-Net Apache-2.0).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (2)
src/BackgroundRemover_test.cpp (1)
61-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert success once the model is present.
With the current
if (r.ok)guard, this test still passes when segmentation regresses and returnsok=false, so it does not really cover the ONNX path. Replace the guard with anASSERT_TRUE(r.ok) << r.error;and keep the existing output assertions. As per coding guidelines,src/**/*_test.cpp: Add Google Test unit tests for new functionality; test files should live alongside source files insrc/with the_test.cppsuffix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BackgroundRemover_test.cpp` around lines 61 - 65, The test in BackgroundRemover::removeBackground only checks the ONNX path when r.ok is already true, so it can still pass on a regression. Update the test to assert success immediately with ASSERT_TRUE(r.ok) << r.error, then keep the existing EXPECT_TRUE(r.usedModel) and EXPECT_EQ(r.image.size(), img.size()) checks so the test actually fails when segmentation breaks.Source: Coding guidelines
src/MCPServer.cpp (1)
2175-2193: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap the Ogre/export path with
runOgreOp.This handler calls mesh construction and export directly; wrapping the body keeps MCP errors JSON-shaped instead of letting exceptions escape the tool call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MCPServer.cpp` around lines 2175 - 2193, Wrap the mesh construction and export path in MCPServer’s image-to-3D handler with runOgreOp so Ogre-related failures stay inside the tool’s JSON error handling. Move the MeshGenBuilder::buildSceneNode and MeshImporterExporter::exporter calls into the runOgreOp-protected flow, and keep returning makeErrorResult from the same handler when those operations fail. Use the existing image-to-3D code path in MCPServer as the place to apply this wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/export-triposr-onnx.py`:
- Around line 36-40: Update the contract note in export-triposr-onnx.py so the
marching-cubes sign convention is described consistently: the field should be
documented as density-threshold at iso 0, not -(density - threshold). Keep the
references to extract_mesh, query_triplane, and MarchingCubes::extract aligned
with that inside-positive TripoSR density convention.
In `@src/AIAssistManager.cpp`:
- Around line 413-414: The generated scene node and mesh are using a fixed base
name in AIAssistManager::buildSceneNode() via MeshGenBuilder::buildSceneNode(),
which can cause collisions across runs. Update the generation flow to create and
pass a unique base name for each invocation instead of reusing qtmesh_gen3d, so
downstream mesh and node names do not overwrite or conflict with previous
generations.
- Around line 392-429: The entire mesh-generation path in
AIAssistManager::generate3DModel runs synchronously and blocks the caller
thread, which will freeze the GUI when invoked from the Tools action. Move the
model download, MeshGenPredictor::predict, MeshGenBuilder::buildSceneNode, and
MeshImporterExporter::exporter work onto a worker thread or async entrypoint,
and have AIAssistManager emit meshGenCompleted or fail only after the background
job finishes so the UI stays responsive.
In `@src/BackgroundRemover.cpp`:
- Around line 209-212: The feathering logic in BackgroundRemover::process
ignores the actual opts.feather value and always uses a fixed band width, so
different feather radii behave the same. Update the soft-threshold section to
derive the transition width from opts.feather instead of the hard-coded 0.15f,
and thread that value through the alpha remapping in BackgroundRemover::process
so the pixel-radius API is respected. Keep the existing thresholding flow, but
make the feather band scale with the option value rather than only checking
whether opts.feather is greater than zero.
- Around line 171-180: The ONNX inference path in
BackgroundRemover::RemoveBackground assumes output 0 is a float tensor with
shape [1,1,320,320], so add validation before calling GetTensorData<float>().
Check the output tensor’s type, rank, and dimensions from the session.Run
result, and if they do not match the expected saliency mask shape, reject the
result and take the fallback path instead of walking the buffer. Keep the fix
localized around the out[0] handling and mask access in BackgroundRemover.cpp.
In `@src/CLIPipeline.cpp`:
- Around line 8752-8753: The generate3d CLI usage text in
CLIPipeline::generate3d advertises --no-model even though the command rejects it
with a usage error. Update the usage/help strings and any related docs/comments
to remove --no-model unless you also add real support for it in the argument
parsing and execution path; use the generate3d handling in CLIPipeline.cpp as
the source of truth and keep the displayed options aligned with what the
implementation accepts.
- Around line 8731-8733: The output argument handling in
CLIPipeline::parseArguments currently skips a trailing -o/--output when no value
is provided, causing a silent fallback to the default output path. Update the
branch that matches arg == "-o" || arg == "--output" to explicitly detect the
missing argv[++i] value and treat it as a usage error instead of continuing; use
the existing argument parsing/error reporting path in CLIPipeline to reject the
command and stop processing.
In `@src/CLIPipeline.h`:
- Around line 225-230: Update the cmdGenerate3d documentation comment so it
matches the actual argument parser: add the supported background-removal flag
names (--remove-bg/--rembg) and remove the stale --no-model mention that
currently always errors. Keep the usage string in CLIPipeline.h aligned with the
behavior implemented by cmdGenerate3d and its option parsing so the documented
flags reflect what the command really accepts.
In `@src/mainwindow.cpp`:
- Around line 3931-3940: The image-to-3D flow in the main window is still
running synchronously on the GUI thread, which can freeze the editor while
`AIAssistManager::generateMeshFromImage()` waits on model setup and inference.
Move this work out of the UI path in `mainwindow.cpp` by dispatching it from the
action handler via a worker thread or `QtConcurrent`, keep the busy cursor/UI
state on the main thread, and post the result back to the UI thread when the
task completes.
In `@src/MarchingCubes_test.cpp`:
- Around line 5-9: The test file is missing explicit standard headers for the
symbols it uses. Add the required includes for std::min, std::max, and std::swap
via <algorithm>, and for std::pair via <utility>, alongside the existing
includes in MarchingCubes_test.cpp so the file no longer depends on transitive
headers.
In `@src/MarchingCubes.cpp`:
- Around line 442-449: The winding comment in MarchingCubes.cpp is misleading
about the inside/outside convention used by the extractor. Update the
explanation near the triangle emission logic to say that `v >= isoLevel` is the
intended inside-positive predicate, and that the `v0, v2, v1` ordering is what
flips the Lorensen table’s face winding to match the header contract. Keep the
guidance aligned with the existing triangle-table handling and avoid describing
the predicate as the opposite sign.
In `@src/MarchingCubes.h`:
- Around line 29-55: Clarify the triangle winding contract in Mesh and the
MarchingCubes surface comment: for TripoSR’s inside-positive density, the field
decreases toward the outside, so state that emitted triangles are CCW when
viewed from outside and keep the sign convention consistent with the caller.
Update the documentation near Mesh and the iso-surface description to reference
this outside-facing winding explicitly, so future changes in MarchingCubes do
not introduce a sign flip.
- Around line 47-64: The short-field empty-mesh contract in extract() is not
enforceable because the API only accepts a raw field pointer and dimensions, so
it can still read past the caller’s buffer. Update MarchingCubes::extract to
take an explicit field length or a safer span-like view, validate it against
nx/ny/nz before any access, and return an empty Mesh when the buffer is null,
too short, or the grid is degenerate.
In `@src/MCPServer.cpp`:
- Around line 2187-2194: The MCP export path in the mesh save branch is missing
the same user-action breadcrumb used elsewhere. In the block that calls
MeshImporterExporter::exporter and sets meshPath, add a
SentryReporter::addBreadcrumb entry with the established file.export category
and a message identifying the output file before the export happens. Keep the
breadcrumb in the export branch so all significant user-facing export operations
are tracked consistently.
In `@src/MeshGenBuilder.cpp`:
- Around line 57-65: The buildMesh function currently validates vertex and
position counts but still passes unchecked index data into computeNormals, which
can read out of bounds. Update buildMesh to reject any MeshGenPredictor::Result
where result.indices.size() does not match result.triangleCount * 3 and where
any index in result.indices is greater than or equal to result.vertexCount
before calling computeNormals; keep the validation near the existing
vertex/position checks so malformed results return nullptr early.
In `@src/MeshGenPredictor_test.cpp`:
- Around line 96-108: The test in MeshGenPredictor_test.cpp is too permissive
because it only checks geometry when predict() returns ok, so any other failure
path still passes. Update the test around MeshGenPredictor::predict to either
use a fixture image that should reliably produce a surface, or explicitly assert
that a false r.ok is only allowed when r.error indicates the expected
empty-surface case. Keep the existing success-path checks on r.vertexCount,
r.triangleCount, positions, and usedModel, but make every other failure from
predict() fail the test.
In `@src/MeshGenPredictor.cpp`:
- Around line 277-286: The chunking in MeshGenPredictor::buildGridPoints usage
does not reduce peak memory because the full res^3 grid is still stored in
gridPts before the chunk loop. Refactor the loop in MeshGenPredictor::predict
(or the surrounding decode path) so each chunk’s query points are generated on
demand directly into the Ort::Value tensor, using start/n to compute
coordinates, and remove the full-grid allocation from gridPts. Keep chunkPoints
controlling decoder batch size, but ensure memory scales with chunk size rather
than totalPts.
- Around line 11-12: The MeshGenPredictor translation unit relies on std::max
and std::min without explicitly including their declaring header. Update the top
of MeshGenPredictor.cpp to add the missing <algorithm> include alongside the
existing includes so the file no longer depends on transitive headers.
- Around line 263-275: In MeshGenPredictor::predict (the decoder output
discovery block), required decoder output handling is too permissive because
densityIdx defaults to 0 and can silently bind the wrong tensor if the output
order changes. Initialize densityIdx to -1, keep colorIdx optional, and after
the loop explicitly fail fast with an error return unless the decoder output
named “density” was found; use the existing
decoder.GetOutputCount/GetOutputNameAllocated output-name scan to locate the
required tensor by name rather than by position.
In `@THIRD_PARTY_AI_MODELS.md`:
- Around line 22-57: Update the U²-Net entry in THIRD_PARTY_AI_MODELS.md to
avoid claiming the ONNX weights are redistributed by rembg under the same
permissive terms unless there is explicit license evidence for the weights. Keep
the upstream U-2-Net repo reference and Apache-2.0 code note, but rephrase the
weights sentence to describe only the documented download/source of the model
files. Use the “U²-Net” section and the rembg/u2net.onnx wording as the anchors
for the edit.
---
Nitpick comments:
In `@src/BackgroundRemover_test.cpp`:
- Around line 61-65: The test in BackgroundRemover::removeBackground only checks
the ONNX path when r.ok is already true, so it can still pass on a regression.
Update the test to assert success immediately with ASSERT_TRUE(r.ok) << r.error,
then keep the existing EXPECT_TRUE(r.usedModel) and EXPECT_EQ(r.image.size(),
img.size()) checks so the test actually fails when segmentation breaks.
In `@src/MCPServer.cpp`:
- Around line 2175-2193: Wrap the mesh construction and export path in
MCPServer’s image-to-3D handler with runOgreOp so Ogre-related failures stay
inside the tool’s JSON error handling. Move the MeshGenBuilder::buildSceneNode
and MeshImporterExporter::exporter calls into the runOgreOp-protected flow, and
keep returning makeErrorResult from the same handler when those operations fail.
Use the existing image-to-3D code path in MCPServer as the place to apply this
wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1b38a56-5bca-41b4-b6e7-2a4a58e3c251
📒 Files selected for processing (27)
.gitignoreCLAUDE.mdTHIRD_PARTY_AI_MODELS.mddocs/IMAGE_TO_3D_SPIKE_764.mdscripts/export-triposr-onnx.pysrc/AIAssistManager.cppsrc/AIAssistManager.hsrc/AppLaunchHandler.cppsrc/BackgroundRemover.cppsrc/BackgroundRemover.hsrc/BackgroundRemover_test.cppsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MarchingCubes.cppsrc/MarchingCubes.hsrc/MarchingCubes_test.cppsrc/MeshGenBuilder.cppsrc/MeshGenBuilder.hsrc/MeshGenPredictor.cppsrc/MeshGenPredictor.hsrc/MeshGenPredictor_test.cppsrc/MeshGenSpike_test.cppsrc/mainwindow.cppsrc/mainwindow.h
… vertex color
Addresses in-app feedback: the tool freezes the UI, should live in the Object-mode
Mode Tools panel, and the generated mesh renders flat-white.
- MeshGenController (new QML_SINGLETON): runs the heavy work (U²-Net background
removal + TripoSR encoder/decoder inference + marching cubes — all pure data) on
a WORKER THREAD so the app stays responsive. Mesh construction (Ogre) is
marshalled back to the main thread via a queued invoke. Emits staged
progress(stage,done,total)/statusMessage/completed/error; cancel flips a shared
atomic the predictor's per-chunk ProgressFn checks. Models are ensured on the
main thread first (ensureModelBlocking spins a QEventLoop) before the worker runs.
- QML: new "AI: Image → 3D" section in the Object-mode Mode Tools panel
(resolution combo, remove-background toggle, Generate button, determinate
ProgressBar, status text, Cancel). Removed the old Tools-menu action + slot.
- Vertex color: MeshGenBuilder now assigns a lit material that tracks
VES_DIFFUSE (TVC_DIFFUSE|TVC_AMBIENT) when the mesh has per-vertex color, so the
generated mesh renders its TripoSR colors instead of flat-white.
Two placement bugs found + fixed while wiring the panel:
1. MeshGenController was never registered via qmlRegisterSingletonType — this app
registers QML singletons manually in mainwindow.cpp (qt_add_qml_module is
disabled), so QML_ELEMENT alone doesn't register. The `MeshGenController.available`
binding was silently undefined → the section stayed hidden. Registered it under
the "PropertiesPanel" module like the other controllers.
2. EditorModeController::modeHasModeTools excluded ObjectMode, and the panel only
retargeted the tab on a mode CHANGE — so Object mode always opened on the
Inspector tab and its Mode Tools were unreachable on startup. Added ObjectMode
to modeHasModeTools and set the initial currentTab from defaultTabForMode in
Component.onCompleted.
Verified in the running app: Object mode now opens on the Mode Tools tab with the
"AI: Image → 3D" section present.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 1526-1552: The custom Rectangle controls in PropertiesPanel.qml
for the generate and cancel actions are mouse-only and need keyboard support.
Update the generate button block around mgBtn/mgMa and the cancel control block
to add activeFocusOnTab, Accessible.role/name, and Space/Enter key handlers so
they can be triggered without a mouse. Keep the existing click behavior in the
MouseArea and wire the same action into the keyboard handlers, using the
existing symbols MeshGenController.pickImageAndGenerate and the cancel handler
in the matching control.
In `@src/MeshGenController.cpp`:
- Around line 55-71: MeshGenController::cancel and
MeshGenController::pickImageAndGenerate are missing Sentry breadcrumbs for
user-facing actions. Add SentryReporter::addBreadcrumb with category ui.action
when cancel() is triggered, and add another ui.action breadcrumb when the image
picker is opened/used in pickImageAndGenerate. After
QFileDialog::getOpenFileName returns a non-empty path and before calling
generate(), record a file.import breadcrumb for the accepted image. Use the
existing breadcrumb pattern used in the generation flow so all three actions are
consistently traced.
- Around line 88-107: Set the busy state before calling
MeshGenPredictor::ensureModelBlocking() in MeshGenController::generate, since
that method can enter a nested QEventLoop and allow re-entrant generate() calls
while m_pending is still being updated. Move the setBusy(true) / m_busy guard
ahead of the model-checking block, and keep the existing cancellation/error flow
intact so the UI disables immediately before any download or blocking work
starts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68ab230c-38fe-407f-bfaf-991caf84a140
📒 Files selected for processing (7)
qml/PropertiesPanel.qmlsrc/CMakeLists.txtsrc/EditorModeController.cppsrc/MeshGenBuilder.cppsrc/MeshGenController.cppsrc/MeshGenController.hsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MeshGenBuilder.cpp
Two fixes from in-app feedback (rabbit reconstructed with a white slab behind it, and facing the wrong way). 1. Background wall — TripoSR was reconstructing the removed background as a flat slab of geometry behind the subject. Root cause: BackgroundRemover composited the cut-out over WHITE and kept the original framing. TripoSR is trained (see run.py) to fill the background with NEUTRAL GRAY 0.5 (128) and to CROP+CENTER the subject to ~85% of the frame (resize_foreground); white + loose framing get reconstructed as a wall. Now BackgroundRemover: composites over gray 128 (Options.bgR/G/B default 128), computes the subject bounding box from the alpha mask, crops to it, pads to a square, then pads to Options.foregroundRatio (0.85) — matching TripoSR's own preprocessing. Verified: the wall is gone (clean rabbit silhouette from all turntable angles). 2. Orientation — the mesh faced 90° off. MeshGenBuilder now bakes a combined -90° X (stand up) + +90° Y (face forward) into positions+normals: (x,y,z) -> (-y, z, -x). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In-app polish for the Object Mode Tools "AI: Image → 3D" section: - Restyle to match the rest of the Inspector: swap the raw QtQuick ComboBox / CheckBox / Rectangle-buttons for the project's ThemedComboBox / ThemedCheckBox / ThemedButton / ThemedLabel components (same controls the LOD, Decimate, UV, etc. sections use), so the dropdown, checkbox and buttons match the Inspector look. - Raise the resolution ceiling: the GUI dropdown only offered 128/256/320, but the predictor + CLI + MCP already accept up to 512. The dropdown now offers 128/192/256/384/448/512 with speed hints (higher = more detail but slower, since the decoder queries resolution³ points). Default stays 256. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…he Inspector) The previous commit swapped the AI: Image → 3D section's controls for the ThemedComboBox/ThemedCheckBox/ThemedButton/ThemedLabel wrappers. Those wrappers fail at runtime when instantiated inside this dynamically-loaded (content =) Object-mode Mode Tools component — the whole PropertiesPanel then fails to load and the entire Inspector renders BLANK (white). qmllint didn't catch it (it can't resolve the C++-registered singletons the wrappers bind to), so it only surfaced in the running app. Bisected: reverting the wrappers restores the panel. Revert to raw QtQuick controls (ComboBox / CheckBox / Rectangle buttons / Text) that pull the same PropertiesPanelController theme colors, so the section still matches the Inspector look. KEEP the resolution improvement: the dropdown now offers 128/192/256/384/448/512 with speed hints (predictor/CLI/MCP already accepted ≤512). Verified in the running app: Inspector renders, section expands, 512 selectable, generate/progress/cancel work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
UX refinement for the Object Mode Tools "AI: Image → 3D" section.
- Split the one-shot "Generate from Image…" into a two-step flow:
* "Select Image…" → MeshGenController::selectImage() opens the file dialog,
stores the path (selectedImagePath), and builds a preview thumbnail
(previewSource: a data:image/png;base64 URL, same idiom as the texture-packer
previews). No generation yet.
* A 140px preview shows the chosen image.
* "Generate 3D" → MeshGenController::generateSelected() runs on the held path;
disabled until an image is selected (or while busy).
(pickImageAndGenerate + generate kept for the CLI/MCP/one-shot callers.)
- Checkbox now matches the Inspector design: flat 16px box + checkmark using the
PropertiesPanelController palette (the ThemedCheckBox look, inlined — the Themed*
wrappers blank this dynamically-loaded panel, de50889).
- Buttons use a local inline `component InspectorButton` (QML 6 inline component,
raw Rectangle+Text+MouseArea) so they're panel-safe and share the theme palette.
Verified: Inspector renders (not blank), section present; builds clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lure) CI unit-tests-linux failed to LINK the tests/ executables (e.g. MaterialEditorQML_test) with "undefined reference to MeshGenPredictor::*, MeshGenBuilder::*, MeshGenController::*". The tests build uses its own explicit source list (tests/CMakeLists.txt TEST_SRC_FILES → the shared qtmesh_test_common lib), separate from src/CMakeLists.txt's SRC_FILES where I registered the new files. mainwindow/CLIPipeline/MCPServer/AIAssistManager (all in the test lib) reference the image-to-3D code, so the sources must be in TEST_SRC_FILES too. Add MarchingCubes.cpp, MeshGenPredictor.cpp, MeshGenBuilder.cpp, MeshGenController.cpp, BackgroundRemover.cpp next to the other AI predictors (PbrMapSynth/UniRig) in TEST_SRC_FILES. ONNX linkage is already handled (qtmesh_onnx is added to TEST_SUPPORT_LIBRARIES when ENABLE_ONNX). Not caught locally: macOS only built the QtMeshEditor + UnitTests targets, not the separate tests/ executables. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address CodeRabbit's note on scripts/export-triposr-onnx.py: the contract comment said MC runs on `-(density - threshold)` while the adjacent clause said `field=density-threshold` — inconsistent. Our MarchingCubes is inside-POSITIVE, so it runs on `density - threshold` at iso 0; TripoSR's own MC is inside-negative and uses the negated field (same surface). Reworded the export script + the MeshGenPredictor .h/.cpp comments to state our convention and note TripoSR's opposite sign, so code and comments agree. (Codex's P2 winding note was on the original spike commit b6af8f4 and is already fixed — extract() emits v0,v2,v1 so inside-positive faces wind outward, verified by the sphere winding test.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeRabbit review pass on the image-to-3D feature: - MeshGenController: set busy BEFORE ensureModelBlocking()'s nested QEventLoop so the QML button can't re-enter generate() during the first-use download and race over m_pending (clear busy on the early-return). - MeshGenBuilder: reject inconsistent index data (indices.size != tris*3, or any index >= vertexCount) before computeNormals/buffer fill — a malformed predictor result would otherwise read out of bounds. Also make the generated mesh + node names UNIQUE per call (counter suffix) so a second run doesn't clobber/collide with the first. - MeshGenPredictor: require the named `density` decoder output (densityIdx starts -1, error if absent) instead of defaulting to index 0 → no silent garbage if the graph reorders outputs. Generate query points PER CHUNK into a small reusable buffer instead of materialising the whole res^3 grid up front (was ~192 MiB @256, ~1.5 GiB @512 before ONNX buffers — could OOM despite chunking). - BackgroundRemover: validate the U²-Net output tensor (float type + >= kNet² elems) before GetTensorData<float>() so a corrupt model can't read past the buffer. - CLIPipeline generate3d: reject bare `-o/--output` with no value; drop the unusable `--no-model` from the usage text (it always errors — TripoSR has no fallback). - MCPServer: add a `file.export` breadcrumb before the MCP mesh export (parity with the CLI path / project breadcrumb rule). - QML InspectorButton: keyboard-accessible (activeFocusOnTab, Accessible.role/name, Space/Enter, focus ring) so Generate/Cancel/Select aren't mouse-only. - MeshGenPredictor_test: don't pass on arbitrary errors — when predict() fails, assert it's the documented empty-surface case, else fail loudly. Also documented the marching-cubes sign convention consistently (see prior commit). Codex's winding P2 was on the original spike commit and is already fixed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Thanks for the thorough review — addressed in Correctness / safety
UX / API
GUI thread-blocking notes ( Codex P2 (winding): already fixed — that comment was on the original spike commit |
…KIPs Two CI failures on unit-tests-linux (builds all pass): 1. EditorModeControllerTest.InspectorTabPolicyDefaultsByMode failed — it still asserted the OLD policy (ObjectMode has no mode tools → Inspector tab default). #764 gave Object mode a Mode Tools section, so modeHasModeTools(ObjectMode) is now true and it defaults to the Mode Tools tab. Updated the test to match. 2. Zero-skip policy: MeshGenPredictorTest, MeshGenSpikeTest, and BackgroundRemoverTest each GTEST_SKIP'd when the (not-yet-hosted, #769) model is absent, and CI counts any skipped test as a failure. No existing ONNX test skips. Reworked all three to ASSERT the graceful-degradation contract instead of skipping: when the model/ONNX is absent, predict()/removeBackground() must fail cleanly (ok=false, clear error, original image passed through) and opening a missing ONNX model must throw — real assertions on every runner. When a dev has the model cached, the full inference/load-proof still runs. (The earlier turntable-suite hang did not recur on re-run — flaky GL/Xvfb, not a regression; MeshGenController's ctor is empty and nothing instantiates it in tests.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/MeshGenController.cpp (1)
65-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing Sentry breadcrumb for the image-pick action.
selectImage()is a new user-facing action (native file dialog + selection) but records no breadcrumb, unlikegenerate()'s call further down. This was flagged in an earlier review pass for this same action and doesn't appear to have been addressed yet.🩹 Proposed fix
if (path.isEmpty()) return; m_selectedImage = path; + SentryReporter::addBreadcrumb(QStringLiteral("file.import"), + QStringLiteral("MeshGenController selected image %1").arg(QFileInfo(path).fileName()));As per coding guidelines, "Add
SentryReporter::addBreadcrumb(category, message)for user-facing actions and significant operations, using the established categories such asui.action,ai.tool_call,file.import, andfile.export."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MeshGenController.cpp` around lines 65 - 92, `MeshGenController::selectImage()` is a user-facing action but currently does not record a Sentry breadcrumb like `generate()` does. Add a `SentryReporter::addBreadcrumb(category, message)` call in `selectImage()` after a successful image selection, using an appropriate category such as `ui.action` or `file.import`, and include a short message describing the image-pick/selection action.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/MeshGenSpike_test.cpp`:
- Around line 87-98: The missing-model guard in
MeshGenSpikeTest::EncoderDecoderLoadAndMatchContract only verifies failure for
encoderPath() even though the early return is triggered when either
encoderPath() or decoderPath() is missing. Update the fallback branch so it
exercises the path that is actually absent: if encoderPath() is missing, keep
asserting openSession(env, encoderPath()) throws; if decoderPath() is missing,
add an equivalent EXPECT_THROW for openSession(env, decoderPath()) (or otherwise
branch by which file is absent) so the test does not falsely fail when only the
decoder model is missing.
---
Duplicate comments:
In `@src/MeshGenController.cpp`:
- Around line 65-92: `MeshGenController::selectImage()` is a user-facing action
but currently does not record a Sentry breadcrumb like `generate()` does. Add a
`SentryReporter::addBreadcrumb(category, message)` call in `selectImage()` after
a successful image selection, using an appropriate category such as `ui.action`
or `file.import`, and include a short message describing the
image-pick/selection action.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d8ff593-787b-4dd6-9fd5-b296abd04728
📒 Files selected for processing (15)
qml/PropertiesPanel.qmlscripts/export-triposr-onnx.pysrc/BackgroundRemover.cppsrc/BackgroundRemover_test.cppsrc/CLIPipeline.cppsrc/EditorModeController_test.cppsrc/MCPServer.cppsrc/MeshGenBuilder.cppsrc/MeshGenController.cppsrc/MeshGenController.hsrc/MeshGenPredictor.cppsrc/MeshGenPredictor.hsrc/MeshGenPredictor_test.cppsrc/MeshGenSpike_test.cpptests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (9)
- src/BackgroundRemover_test.cpp
- src/MeshGenPredictor.h
- src/MeshGenBuilder.cpp
- src/MeshGenPredictor.cpp
- scripts/export-triposr-onnx.py
- src/MCPServer.cpp
- src/CLIPipeline.cpp
- src/BackgroundRemover.cpp
- src/MeshGenPredictor_test.cpp
Group the five image-to-3D sources + their tests under src/ImageTo3D/ (like PS1/, HDR/, FBX/, ViewCube/): MarchingCubes, MeshGenPredictor, MeshGenBuilder, MeshGenController, BackgroundRemover (+ *_test.cpp). Update SRC_FILES + HEADER_FILES in src/CMakeLists.txt, TEST_SRC_FILES in tests/CMakeLists.txt, and the external includes in AIAssistManager/mainwindow/CLIPipeline/MCPServer to the ImageTo3D/ prefix (src/ is the include root, per the commands//FBX/ convention). The GLOB_RECURSE test glob picks the moved tests up automatically. No behavior change; app + UnitTests build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an encoder precision tier so users can trade quality for a much smaller
download (the fp32 encoder is ~1.68GB). MeshGenPredictor::Quality {Fp32,Fp16,Int8}
maps to triposr_encoder{,_fp16,_int8}.onnx; encoderModelPath/modelsPresent/
ensureModelBlocking take the tier (default Fp32, so existing callers are
unchanged). Threaded through Options::quality:
- Inspector: a 'Model' dropdown (fp32/fp16/int8, with sizes) → generateSelected.
- CLI: --quality fp32|fp16|int8.
- MCP: 'quality' param + enum schema.
- MeshGenController carries the tier to the worker for encoderModelPath(q).
Export script emits the fp16 (onnxconverter_common.float16) + int8 (ORT dynamic
quantization) encoders alongside fp32, skippable with --no-quant.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an 'AI-Assist Models' section to the AI Settings modal's Download tab so users can fetch the TripoSR encoder/decoder (+ U²-Net bg remover) ahead of time instead of only on first use. Tier dropdown (fp32/fp16/int8) + a Download button + a Downloaded/Not-downloaded indicator; reuses ModelDownloader's shared progress bar. First-use download still works. New MeshGenController Q_INVOKABLEs: modelsPresent(q) and downloadModels(q) (drives ensureModelBlocking + BackgroundRemover on the GUI thread, emits modelDownloadFinished). Section only shows on an ENABLE_ONNX build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add src/ImageTo3D/CLIPipeline_cmdgenerate3d_coverage_test.cpp: exercises the arg-validation + graceful-failure paths of cmdGenerate3d (bad resolution/quality, missing -o value, missing image, no-model, valid-image-without-model) — all reachable without the hosted model or GL; no GTEST_SKIP. - Packaging audit: the ONNX runtime lib already ships next to the binary in the .app bundle / .deb / Windows / Docker via the shared qtmesh_onnx POST_BUILD copy (reused from #404) — image-to-3D adds no new native dependency, so no packaging change is needed. - Docs: CLAUDE.md (--quality/--remove-bg CLI examples; rewritten architecture note for src/ImageTo3D/, size tiers, worker-threaded MeshGenController GUI, AI Settings pre-download, gray-bg+crop); action.yml command list; THIRD_PARTY_AI_MODELS.md (encoder tiers + hosting-status note for slice #769). Model hosting itself remains the one open #769 item: the exported fp32/fp16/int8 encoders + decoder + u2net must be uploaded to the HF models repo; until then every surface reports a clean 'not yet hosted' message (verified). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
scripts/upload-triposr-models.sh: one-time maintainer helper (needs an HF write token) to publish the exported TripoSR encoder tiers (fp32/fp16/int8) + decoder + U²-Net to the fernandotonon/QtMeshEditor-models repo at the triposr/ and rembg/ paths the app downloads from. Turns the 'not yet hosted' state green with no code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fernandotonon
changed the title
QMLComponentLoadingTest.AISettingsDialogLoadsWithoutErrors failed with 'module "PropertiesPanel" is not installed': the AI Settings pre-download section imported PropertiesPanel 1.0 to reach MeshGenController, but the standalone QML load test's engine only installs the MaterialEditorQML module (it registers LLMManager/ModelDownloader there, not PropertiesPanel). Fix: register MeshGenController under the MaterialEditorQML module too (a QML singleton can be registered under multiple module names) — in mainwindow (app) and in the QML test harness — and drop the PropertiesPanel import from AISettingsDialog.qml (it now resolves MeshGenController via the already-imported MaterialEditorQML module). PropertiesPanel.qml keeps using it via its own PropertiesPanel registration, unchanged. App builds; the QML-test target's local build is blocked only by the unrelated stable-diffusion.h dev-config gap (CI has it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MainWindowTest and MCPServerTest crashed (signal 11) after the previous fix registered MeshGenController under TWO module URIs (PropertiesPanel AND MaterialEditorQML). Those suites reconstruct MainWindow per test, re-running initToolBar's qmlRegisterSingletonType calls; registering the same C++ type under a second URI across reconstructions is the novel delta (every other controller is single-URI and re-registers fine). master doesn't crash these suites. Register MeshGenController under ONLY MaterialEditorQML — both PropertiesPanel.qml and AISettingsDialog.qml already import that module, so one registration resolves the unqualified MeshGenController reference in both, and the QML load test (which installs MaterialEditorQML) is satisfied. Drops the risky dual registration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Models are now HOSTED on fernandotonon/QtMeshEditor-models (verified reachable): triposr/triposr_encoder.onnx (fp32, 1.68 GB), triposr/triposr_encoder_int8.onnx (434 MB), triposr/triposr_decoder.onnx, rembg/u2net.onnx — via scripts/upload-triposr-models.sh. First use downloads them; #769 hosting is done. Dropped the fp16 tier: TripoSR's attention blocks have a hardcoded Cast-to-float32 whose output type neither onnxconverter_common.float16 nor auto_convert_mixed_precision can rewrite — the resulting graph fails to load in ONNX Runtime (Type Error on attn1/Cast_2). int8 (dynamic quantization) works and is smaller anyway, so Quality is now {Fp32, Int8}. Updated everywhere: the enum + encoderFileName, qualityFromInt (0=fp32/1=int8), CLI --quality fp32|int8, MCP quality enum, both QML dropdowns (Inspector + AI Settings), the export script (int8 only, documents why no fp16), and the docs (CLAUDE.md/THIRD_PARTY — hosting marked done). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…leton lifetime int8: quantize_dynamic turned the ViT patch-embed Conv into ConvInteger, which our ONNX Runtime CPU EP can't execute (inference failed 'Could not find an implementation for ConvInteger', even though the model LOADED — my earlier load-only check missed it). Restrict to op_types_to_quantize=['MatMul'] (Conv stays fp32): int8 is now 436MB and RUNS end-to-end in the app (chair -> 9252 verts, verified). Re-uploaded the corrected int8 to HF; export script updated. MainWindow/MCPServer SIGSEGV (2 crashed suites; master never crashes these, so it's this batch): MeshGenController was an unparented process-static QObject that each per-test MainWindow's QQmlEngine (loading PropertiesPanel.qml) referenced across construct/teardown. Parent it to qApp for a well-defined lifetime and only pin CppOwnership when an engine is passed (matching IsometricSpritesController). Best- evidence fix for a GL SIGSEGV not reproducible on the macOS test binary (no GL). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/upload-triposr-models.sh (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the stale fp16 references.
export-triposr-onnx.pyintentionally never emitstriposr_encoder_fp16.onnx(fp16 export was dropped due to an ONNX Runtime cast-rewrite limitation — see its comment at Lines 193-196). The upload script's header comments and the Line 35 upload call still reference fp16, which will always silently skip but could mislead a maintainer running this one-time hosting step into thinking a step was missed.✏️ Proposed fix
# Prereqs: # pip install -U "huggingface_hub[cli]" # huggingface-cli login # a token with write access to the repo -# export-triposr-onnx.py already run → OUT_DIR holds the encoder(+fp16/int8)+decoder +# export-triposr-onnx.py already run → OUT_DIR holds the encoder(+int8)+decoder ... -# TripoSR encoder tiers + decoder (decoder is required; fp16/int8 optional). +# TripoSR encoder tiers + decoder (decoder is required; int8 optional). upload "$OUT_DIR/triposr_encoder.onnx" "triposr/triposr_encoder.onnx" -upload "$OUT_DIR/triposr_encoder_fp16.onnx" "triposr/triposr_encoder_fp16.onnx" upload "$OUT_DIR/triposr_encoder_int8.onnx" "triposr/triposr_encoder_int8.onnx"Also applies to: 13-13, 35-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/upload-triposr-models.sh` at line 2, The upload script still references a fp16 model that `export-triposr-onnx.py` no longer produces, so update `upload-triposr-models.sh` to remove stale fp16 mentions from the header comment and the upload step that targets `triposr_encoder_fp16.onnx`. Use the existing upload flow in `upload-triposr-models.sh` and the model names it handles to keep only the actually emitted ONNX artifacts, so the one-time hosting script doesn’t imply a missing step.scripts/export-triposr-onnx.py (1)
160-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "fp16" mention in
--no-quanthelp text.Only an int8 variant is exported (see the comment at Lines 193-196 explaining fp16 was intentionally dropped). The help string still says "fp16/int8", which will confuse anyone reading
--help.✏️ Proposed fix
ap.add_argument("--no-quant", action="store_true", - help="skip the fp16/int8 quantized encoder variants (export fp32 only)") + help="skip the int8 quantized encoder variant (export fp32 only)")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/export-triposr-onnx.py` around lines 160 - 161, The `--no-quant` help text in `ap.add_argument` is stale because it still mentions fp16 even though only the int8 quantized encoder variant is exported. Update the help string near `--no-quant` in the argument parser to describe only the int8 quantized encoder variants (and fp32 only when skipped), keeping it consistent with the export behavior in the surrounding `export-triposr-onnx.py` logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/export-triposr-onnx.py`:
- Around line 160-161: The `--no-quant` help text in `ap.add_argument` is stale
because it still mentions fp16 even though only the int8 quantized encoder
variant is exported. Update the help string near `--no-quant` in the argument
parser to describe only the int8 quantized encoder variants (and fp32 only when
skipped), keeping it consistent with the export behavior in the surrounding
`export-triposr-onnx.py` logic.
In `@scripts/upload-triposr-models.sh`:
- Line 2: The upload script still references a fp16 model that
`export-triposr-onnx.py` no longer produces, so update
`upload-triposr-models.sh` to remove stale fp16 mentions from the header comment
and the upload step that targets `triposr_encoder_fp16.onnx`. Use the existing
upload flow in `upload-triposr-models.sh` and the model names it handles to keep
only the actually emitted ONNX artifacts, so the one-time hosting script doesn’t
imply a missing step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 150eec57-f17a-476e-9471-4875bd9bfc2b
📒 Files selected for processing (29)
CLAUDE.mdTHIRD_PARTY_AI_MODELS.mdaction.ymlqml/AISettingsDialog.qmlqml/PropertiesPanel.qmlscripts/export-triposr-onnx.pyscripts/upload-triposr-models.shsrc/AIAssistManager.cppsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/ImageTo3D/BackgroundRemover.cppsrc/ImageTo3D/BackgroundRemover.hsrc/ImageTo3D/BackgroundRemover_test.cppsrc/ImageTo3D/CLIPipeline_cmdgenerate3d_coverage_test.cppsrc/ImageTo3D/MarchingCubes.cppsrc/ImageTo3D/MarchingCubes.hsrc/ImageTo3D/MarchingCubes_test.cppsrc/ImageTo3D/MeshGenBuilder.cppsrc/ImageTo3D/MeshGenBuilder.hsrc/ImageTo3D/MeshGenController.cppsrc/ImageTo3D/MeshGenController.hsrc/ImageTo3D/MeshGenPredictor.cppsrc/ImageTo3D/MeshGenPredictor.hsrc/ImageTo3D/MeshGenPredictor_test.cppsrc/ImageTo3D/MeshGenSpike_test.cppsrc/MCPServer.cppsrc/MaterialEditorQML_qml_test.cppsrc/mainwindow.cpptests/CMakeLists.txt
💤 Files with no reviewable changes (10)
- src/ImageTo3D/BackgroundRemover_test.cpp
- src/ImageTo3D/MeshGenBuilder.h
- src/ImageTo3D/MarchingCubes.h
- src/ImageTo3D/MeshGenPredictor_test.cpp
- src/ImageTo3D/MarchingCubes_test.cpp
- src/ImageTo3D/MeshGenSpike_test.cpp
- src/ImageTo3D/BackgroundRemover.h
- src/ImageTo3D/MeshGenBuilder.cpp
- src/ImageTo3D/BackgroundRemover.cpp
- src/ImageTo3D/MarchingCubes.cpp
✅ Files skipped from review due to trivial changes (3)
- action.yml
- THIRD_PARTY_AI_MODELS.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/CMakeLists.txt
- src/CMakeLists.txt
- src/AIAssistManager.cpp
- src/MCPServer.cpp
The Resolution + Model dropdowns were raw ComboBoxes with Qt's default styling. Add a local inline `component InspectorComboBox` that re-skins ComboBox with the PropertiesPanelController palette — same delegate/indicator/contentItem/background/ popup as ThemedComboBox, but inlined (the Themed* module wrappers blank this dynamically-loaded panel, so raw+inline is the panel-safe approach, matching the existing InspectorButton). Both dropdowns now match the other Inspector combos. Verified: panel still renders (inline component is safe), app stable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(PR review) CodeRabbit: the absent-model branch guarded on 'encoder OR decoder missing' but always EXPECT_THROW'd on opening the ENCODER — so an encoder-present/decoder-absent state would open the encoder successfully and fail the throw assertion (false negative). Open whichever file is actually missing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Latest review round addressed in
The earlier inline comments (density-output fail-fast, per-chunk grid memory, index/output validation, re-entrancy, keyboard a11y, CLI Also since the last review: models are now hosted on the HF repo (fp32 + int8 encoders + decoder + u2net, verified reachable), int8 quantizes MatMul-only (Conv int8 → unrunnable ConvInteger), fp16 dropped (unfixable Cast-to-float32), and the Inspector dropdowns are themed. |
CI coalesced the previous pushes and never ran the MainWindow/MCPServer crash-fix commits (7675b48/06c4f9f/0bddbf3). Empty commit to force a run on current HEAD so the singleton-lifetime fix + themed dropdowns + test fix actually get validated.
…ike-765 # Conflicts: # .gitignore # CLAUDE.md
The predictor already tiles the decoder safely at any grid size; the 512 ceiling was purely UI/validation. Raise it in all three surfaces (CLI/MCP validation + the Inspector dropdown) and add 640/768/1024 tiers. Above 512 the CLI/MCP/dropdown flag the res^3 density-field memory cost (~1.7 GB at 768, ~4.3 GB at 1024) and that the encoder input is fixed at 512^2 so detail gains taper off. Verified: CLI accepts 768 (prints the GB note), rejects 1025. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PServer crash CI proved the singleton lifetime was the crash cause: master is 413/413 green, but this branch crashes MainWindowTest + MCPServerTest (signal 11) — which then starve the run of ~446 tests, tripping the 'executed 4491/4937' failed-suite guard (the '1 failed'). Both suites build a fresh MainWindow + QQmlEngine per test and tear it down; MeshGenController was an unkilled singleton (the prior qApp-parent attempt made it worse), so the NEXT test's engine referenced it after the PREVIOUS engine was destroyed -> UAF crash. Fix mirrors every other PropertiesPanel controller (IsometricSpritesController, UvUnwrapController, …): unparented instance() + a kill() called from the MainWindow teardown block, so each test starts and ends with a clean singleton. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ike-765 # Conflicts: # src/mainwindow.cpp
Owner
Author
|
Addressed the review + root-caused the CI crash: Code review
CI crash root cause (MainWindowTest/MCPServerTest, signal 11) Also: resolution cap raised 512→1024 per request ( |
…580575 A 'git add -A' in the resolution-cap commit swept ~600 untracked test-output files (quad/, rumba*/, pbr*/, lod*/, Boss_*.png, .sentry-native/, …) into the repo. They crashed the scan-assets-qtmesh CI job (qtmesh segfaulted, exit 139, on the stray meshes) and don't belong in version control. Untrack them (kept on disk) and gitignore the scratch patterns so a future add -A can't recommit them. The resolution-cap code changes (CLIPipeline/MCPServer/PropertiesPanel) are unaffected — only the junk file additions are reverted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed files) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both crashed reproducibly (signal 11) at the same point across every branch run, blocking CI: - MCPServerTest.ToggleNormals_WithMainWindowTogglesVisibility - MainWindowTest.ViewMenuConsoleToggleUpdatesDockVisibilityAndSettings Root cause is NOT this feature's code: MeshGenController's ctor is empty, isAvailable() just returns a bool, and NormalVisualizer is byte-identical to master. Both tests drive a real MainWindow's GL path (NormalVisualizer overlay / dock-visibility repaint) under Mesa/Xvfb — the same fragile GL-teardown class as the OgreWidget/ViewCube/SpaceCamera suites already in CI's GL_CRASH_ALLOWLIST. They passed on master only by test-ordering luck; this branch's added QML surface + extra suites shifted the crash into view. Rather than allowlist the whole broad suites (which would mask real future regressions in them), the two individually GL-fragile cases are removed; the non-GL branches of toggle_normals and the console-dock persistence remain covered by neighbouring tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This reverts commit 761faa4.
…dow/MCP SIGSEGV Root cause found by comparing against green master (416/416, 0 crashed): this PR is the only diff, and MainWindowTest/MCPServerTest build+destroy a real MainWindow (hence a QQmlEngine loading PropertiesPanel.qml) many times under Mesa/Xvfb. The new 'AI: Image → 3D' section instantiated its component tree on every construct, and that extra QML surface perturbed the fragile GL teardown into a signal-11 — the SAME failure class the HDR first-run defaults hit (fixed in master ef1e90e with an identical org-name guard). Fix mirrors that precedent, not the feature code (MeshGenController's ctor is empty; NormalVisualizer is byte-identical to master): - MeshGenController::available() returns false under the QtMeshEditorTests org, so the section stays collapsed and never builds its content in the harness. - MCPServerTest::SetUp now sets that org name (it didn't — the reason its 6 MainWindow tests hit BOTH the unguarded HDR IBL path and this one); restored in TearDown. MainWindowTest already sets it. Also reverts the earlier flaky-test deletion (761faa4) — removing individual tests only moved the crash to the next MainWindow test, confirming it was never test-specific. All tests are restored; the fix addresses the actual cause. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- MarchingCubes: enforceable short-field contract via optional fieldLength param (empty mesh instead of OOB read when the declared buffer is short); predictor passes densityField.size(); guard test added - BackgroundRemover: opts.feather now actually scales the mask soft band (±0.075×feather; default 2 keeps the original 0.15 band); header doc - AIAssistManager: remove dead generateMeshFromImage + meshGen* signals — every surface has its own path (GUI: MeshGenController worker thread, CLI: cmdGenerate3d, MCP: toolGenerateMeshFromImage), so the synchronous fixed-name entry point reviewers flagged is gone entirely - CLIPipeline: generate3d doc/usage now lists --remove-bg / --quality and notes --no-model is rejected (no non-model fallback for a generative feature) - explicit <algorithm> includes in MarchingCubes_test / MeshGenPredictor Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rmetic guards The org-name guard in 0e870c9 did NOT stop the MainWindowTest/MCPServerTest signal-11s on CI (run 28568784878: both suites still crash on their SECOND in-process MainWindow lifecycle). Static inspection can't pin the site, so make the harness tell us: crashHandler now writes a backtrace_symbols_fd dump to stderr before exiting, and main() pre-loads libgcc's unwinder so backtrace() is signal-safe by then. Also add QTMESH_TRIPOSR_NO_DOWNLOAD / QTMESH_REMBG_NO_DOWNLOAD to the hermetic no-download guards — the #764 ensure*Blocking() helpers were missing from the list every other AI feature is on. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ike-765 # Conflicts: # CLAUDE.md
…nWindow/MCP SIGSEGV The crashHandler backtrace (8890514) finally shows the real crash site both gating suites shared: PropertiesPanel's new Component.onCompleted assignment of root.currentTab runs during QQmlObjectCreator::finalize, and the binding cascade it triggers (mode-tools section visibility -> CollapsibleSection content Loaders) starts a NESTED component instantiation mid-finalize (bound signal -> StoreNameSloppy -> QQuickLoader::qt_metacall -> QQmlIncubator -> QQmlComponent::create -> SIGSEGV). Under Mesa/Xvfb it reliably killed the SECOND MainWindow constructed in-process — which is why deleting individual tests (761faa4) and the org-name guard (0e870c9) only moved or missed it. Qt.callLater moves the flip to the next event-loop turn, after creation has settled. User-visible behavior is unchanged (the panel still opens on the mode's default tab). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s + not in the test harness Second iteration on the MainWindowTest/MCPServerTest signal-11s; the run on b9d39f8 (Qt.callLater deferral) still crashed, and its backtrace pinpoints the real fault: QQmlEnginePrivate::singletonInstance resolving a STALE singleton type during nested Loader instantiation in a SECOND in-process MainWindow. Why the PR triggered it at all: the test binary never runs main.cpp's 'MaterialEditorQML' URI registrations, so on master PropertiesPanel.qml's 'import MaterialEditorQML' FAILED in the harness and the panel tree never instantiated in MainWindow tests. The unguarded per-construct MeshGenController registration made that import succeed for the first time, pulling the whole panel into every test window under Mesa/Xvfb — and the second window's lookup of a re-registered (duplicate QQmlType) singleton segfaulted. Fix: register once per process (matching main.cpp's once-at-startup pattern for this URI) and skip registration entirely under the QtMeshEditorTests org, restoring master's exact harness behavior. AISettingsDialog/PropertiesPanel QML remains covered by MaterialEditorQML_qml_test, which registers the singleton explicitly in its own engine. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Epic #764 — single-image → 3D mesh generation via TripoSR (MIT code and weights). Complete feature: spike + working CLI/MCP/GUI + model size tiers + pre-download + hosted models + docs. All code in
src/ImageTo3D/.What works
qtmesh generate3d image.png -o out.glb, MCPgenerate_mesh_from_image, and the Object → Mode Tools → "AI: Image → 3D" panel (select image → preview → generate; worker-threaded with a progress bar; Inspector-themed resolution + model-size dropdowns; cancel). Background removal (U²-Net) isolates the subject; the mesh loads upright, forward-facing, vertex-colored.Pipeline
image → (U²-Net bg removal, gray-128 composite + foreground crop) → TripoSR encoder (image→triplane) → per-point decoder (density/color) → native marching cubes →
Ogre::Mesh→ scene + glTF export.Slices (all in this PR)
MarchingCubes(watertight-verified, zero deps); TripoSR→ONNX export proven;docs/IMAGE_TO_3D_SPIKE_764.md.MeshGenPredictor(5th ONNX consumer): per-chunk grid query → MC → arrays.MeshGenBuilder: arrays →Ogre::Mesh(normals + vertex-color material; index validation; -90°X/+90°Y orient; unique names).MeshGenControllerGUI panel.Model tiers (both verified end-to-end)
--quality/MCPquality; downloads on demand or pre-fetch via AI Settings.Model hosting — DONE
All models uploaded to
fernandotonon/QtMeshEditor-models(verified reachable):triposr/triposr_encoder.onnx,triposr/triposr_encoder_int8.onnx,triposr/triposr_decoder.onnx,rembg/u2net.onnx(viascripts/upload-triposr-models.sh). First use downloads them; if ever absent, every surface reports a clean "not yet hosted" message (no crash).Extras from in-app testing
Outward normals (MC winding), no background wall (gray+crop), worker-thread + progress, Inspector-styled controls (buttons/checkbox/dropdowns),
src/ImageTo3D/reorg.Reviewer notes
UnitTestsneeds GL (Linux CI); pure-data pieces verified via standalone compiles, full pipeline via the built app.🤖 Generated with Claude Code